security: redact sensitive data in logger decorators + httpclient logging#121
Conversation
…ging Fixes GitHub CodeQL go/clear-text-logging HIGH alerts: 1. logger_decorator.go — strengthen sanitizeLogArgs with a broad case-insensitive sensitive-key list (password, passwd, secret, token, authorization, auth, apikey, api_key, accesskey, access_key, credential, cookie, set-cookie, private_key, privatekey, session, bearer) via isSensitiveKey helper. Apply sanitizeLogArgs in DualWriterLoggerDecorator (both inner + secondary), ValueInjectionLoggerDecorator (on combined args), FilterLoggerDecorator, LevelModifierLoggerDecorator.logWithLevel, and PrefixLoggerDecorator (already had it). BaseLoggerDecorator remains a pure passthrough to preserve the MaskingLogger (logmasker) contract. 2. httpclient/module.go — add isSensitiveHeader + redactHeaders helpers that replace values of Authorization, Proxy-Authorization, Cookie, Set-Cookie, X-Api-Key, X-Auth-Token, and any header containing token/secret/password/ apikey with "***". Apply redactHeaders at both logRequest (~line 695) and logResponse (~line 826) important_headers logging sites. Tests added in logger_decorator_test.go (TestSanitizeLogArgs_BroadSensitiveKeys, TestSanitizeLogArgs_NonSensitivePassThrough, TestDecorators_SensitiveArgsAreMasked) and httpclient/logging_improvements_test.go (TestSensitiveHeadersRedacted). All existing tests continue to pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📋 API Contract Changes Summary✅ No breaking changes detected - only additions and non-breaking modifications Changed Components:Core FrameworkContract diff saved to artifacts/diffs/core.json Module: authContract diff saved to artifacts/diffs/auth.json Module: cacheContract diff saved to artifacts/diffs/cache.json Module: databaseContract diff saved to artifacts/diffs/database.json Module: eventbusContract diff saved to artifacts/diffs/eventbus.json Module: jsonschemaContract diff saved to artifacts/diffs/jsonschema.json Module: letsencryptContract diff saved to artifacts/diffs/letsencrypt.json Module: reverseproxyContract diff saved to artifacts/diffs/reverseproxy.json Artifacts📁 Full contract diffs and JSON artifacts are available in the workflow artifacts. |
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…king Address adversarial-review findings on the go/clear-text-logging fix: CRITICAL 1 — httpclient detailed-logging path leaked secrets. When LogHeaders/LogBody is enabled, logRequest/logResponse logged the raw httputil dump (Authorization/Cookie/Set-Cookie in clear text) as "details". Added redactDump(string) which scrubs sensitive header values in the dump's header section (up to the first blank line) to "***" while preserving header names, the request/status line, and the body. Applied to both detailed logRequest and logResponse before logging; truncation now operates on the already-redacted string. CRITICAL 2 — over-masking tenantID. "tenant"/"requestid" moved to an exact-match set (sensitiveKeyExact) so tenantID/tenantName/tenantCount (primary observability keys) are no longer masked. IMPORTANT 3+4 — bare auth/token/key substrings over-masked author/authority/authenticated/token_count/primary_key. Replaced the broad Contains list with a precise sensitiveKeySubstrings set (password, passwd, secret, credential, apikey/api_key/api-key, accesskey/access_key, privatekey/private_key, authorization, cookie, bearer, and explicit *_token compound forms). Bare auth/token/key removed. Tests: TestSanitizeLogArgs_SensitiveKeys (precise masks), TestSanitizeLogArgs_NotMasked (over-masking regression guard: tenantID/token_count/author/authority/etc.), TestSensitiveHeadersRedactedInDetailedDump (LogHeaders=true dump redaction), and updated existing detailed-logging assertion to require the Authorization secret VALUE be absent while the header NAME remains. IMPORTANT 5 — residual CodeQL taint through the dump string is expected post-rescan and will be dismissed by the lead; the runtime leak is now structurally redacted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Fixes GitHub CodeQL go/clear-text-logging HIGH-severity alerts in the root
modularmodule.sanitizeLogArgswith a broad case-insensitive sensitive-key list (password,passwd,secret,token,authorization,auth,apikey,api_key,accesskey,access_key,credential,cookie,set-cookie,private_key,privatekey,session,bearer). AddedisSensitiveKeyhelper for the matching logic. AppliedsanitizeLogArgsin every decorator that forwards raw caller args to a logging sink:DualWriterLoggerDecorator(bothinner+secondary),ValueInjectionLoggerDecorator(on combined args, after injection),FilterLoggerDecorator,LevelModifierLoggerDecorator.logWithLevel, andPrefixLoggerDecorator(already had it).BaseLoggerDecoratorremains a pure passthrough to preserve theMaskingLogger(logmasker subpackage) contract and avoid double-masking.isSensitiveHeaderandredactHeadershelpers. Sensitive headers (Authorization, Proxy-Authorization, Cookie, Set-Cookie, X-Api-Key, X-Auth-Token, and any header whose name containstoken/secret/password/apikey) have their values replaced with***before being logged asimportant_headers. Applied at both thelogRequestandlogResponsesites.What changed
logger_decorator.gosensitiveKeyPatternslist +isSensitiveKeyhelper;sanitizeLogArgsuses contains-match (case-insensitive); sanitization applied inDualWriter,ValueInjection,Filter,LevelModifierdecoratorslogger_decorator_test.goTestSanitizeLogArgs_BroadSensitiveKeys(per-key subtests),TestSanitizeLogArgs_NonSensitivePassThrough,TestDecorators_SensitiveArgsAreMasked(per-decorator subtests)httpclient/module.gosensitiveHeaderPatterns,isSensitiveHeader,redactHeadershelpers; applied at both request and response logging siteshttpclient/logging_improvements_test.goTestSensitiveHeadersRedacted: asserts Authorization/Cookie/Set-Cookie values are masked in loggedimportant_headersTest plan
GOWORK=off go test -race ./...(root) — okcd httpclient && GOWORK=off go test -race ./...— okGOWORK=off go vet ./...— cleanGOWORK=off golangci-lint run ./...(root + httpclient) — 0 issuesgofmt -lon changed files — cleanNotes
LogHeaders=true/LogBody=true, which dumps raw HTTP viahttputil.DumpRequestOut) are out of scope for this PR — those paths intentionally expose full wire content when explicitly enabled by the operator. Those will be triaged separately by the lead.logmaskersubpackage (MaskingLogger) applies its own[REDACTED]strategy before reachingBaseLoggerDecorator, which is whyBaseLoggerDecoratorremains a pure passthrough — adding sanitization there would cause double-masking and break existinglogmaskertests/behavior.🤖 Generated with Claude Code